Skip to content

Upgrading to Pydantic v2 and DiffSync v2 #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Upgrading to Pydantic v2 and DiffSync v2 #327

wants to merge 11 commits into from

Conversation

@theandrelima
Copy link

@theandrelima theandrelima commented Jul 4, 2024 •
edited
Loading

Overview

network-importer's dependency on pydantic = "^1.6" limits users who need to use other packages dependent on Pydantic 2.x in the same Python environment.

The original motivation behind this PR was to move network-importer's dependency to Pydantic v2.x. However, network-importer depends on DiffSync, which in turn also depends on Pydantic.

For that, moving to DiffSync v2 is also required for compatibility with Pydantic v2.

Changes Made

  • Upgraded to Pydantic v2.
  • Refactored settings classes to use pydantic-settings.
  • Applied overall code fixes for Pydantic v2 compatibility.
  • Upgraded to DiffSync v2 and made necessary code adjustments for breaking changes.
  • Proposed project version bump to 4.0.0 (subject to NTC's discretion) due to breaking changes.

Testing

  • All tests passed using the invoke pytest command. (see Additional Notes below)
  • Manual testing in CLI mode against a dev instance of our Nautobot environment confirmed the functionality both in check and apply modes, and with/without --update-configs and --limit flags.

Additional Notes

invoke pytest only worked after I added the following line to the end of the Dockerfile: RUN pip install .

I'm not sure why, but lines 14 and 15 of the current Dockerfile were not taking the effect of making network_importer itself available in the python environment of the resulting built image.

I decided to not tackle this at all within this PR because:
1 - It might be some weirdness with my own sandbox environment.
2 - Even if it is not, I believe it is out-of-scope.

@msheiny
Copy link

msheiny commented Dec 24, 2024

Looks like this is failing because of the removed docstrings in BatfishSettings and NetworkSettings classes:

./network_importer/config.py:41 in public class `BatfishSettings`:
        D101: Missing docstring in public class
./network_importer/config.py:51 in public class `NetworkSettings`:
        D101: Missing docstring in public class

@jvanderaa
Copy link
Contributor

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

@theandrelima
Copy link
Author

theandrelima commented Jan 1, 2025 •
edited
Loading

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

I was off for a couple of weeks. Back now and happy to see that this might get merged soon, hopefully?

All right! tWill do.
But just to confirm, that is to happen in a separate PR, right?

@theandrelima
Copy link
Author

theandrelima commented May 19, 2025 •
edited
Loading

It seems the idea is to deprecate network-importer as a standalone package in favour of the new onboarding plugin, which will include network importing functionality too.

Whereas that is nice, I'm a big proponent of an independent NI package that can be integrated in whichever way best suits each use case. For example, I currently would love to have NI as a service with a backend REST API, dedicated workers, and running independently of Netbox/Nautobot app, while updating it.

I'll keep my own version of this package, possibly with a different name too. Hope that's not a problem...

cheers...
🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chadell chadell Awaiting requested review from chadell

@glennmatthews glennmatthews Awaiting requested review from glennmatthews glennmatthews is a code owner

@jvanderaa jvanderaa Awaiting requested review from jvanderaa jvanderaa is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@theandrelima @msheiny @jvanderaa